Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix multi-target MLJFlux example #183

Closed
wants to merge 2 commits into from

Conversation

mohamed82008
Copy link

Hi! Thanks for this really cool package. I noticed the multi-target MLJ example in this PR is not working because of an eltype error in measurements vector which triggers an error when the report is generated. I "fixed" it here in this PR and added a test but I am not sure if I was doing something wrong to begin with to trigger this error or not as I am still a novice in MLJ. Please let me know if my example is wrong for some reason or if the fix here is appropriate.

@mohamed82008
Copy link
Author

Turns out I messed up the definition of the multi-target loss. So this PR is not necessary at all.

@ablaom
Copy link
Member

ablaom commented Sep 5, 2022

@mohamed82008 Thanks for spending time with this packages and going to the trouble of contributing a PR.

I wonder if your confusion might be something others will also trip up on. If you can say a few words about what the confusion was, I will open an issue to address this in future doc fixes.

@mohamed82008
Copy link
Author

The problem is that most loss functions don't support multiple targets by default so when using multi-target regression from MLJFlux, I have to either use their built-in losses or roll my own implementation which I did to be able to try out more loss functions, e.g. from LossFunctions.jl. The problem is that the implementation of the multi_target function used in the tests in this PR was wrong so it was returning a vector sometimes. Somehow this didn't break the optimisation but it broke the report making. The following implementation made things work:

function multi_target(loss)
  function _loss(x1::Real, x2::Real)
    return loss(x1, x2)
  end
  function _loss(x1, x2::NamedTuple)
    sum(map(x1, x2) do _x1, _x2
        sum(loss(_x1, _x2))
    end)
  end
  function _loss(x1::Matrix, x2::Matrix)
    sum(loss(vec(x1), vec(x2)))
  end
  return _loss
end

Perhaps my life would have been easier if this vector-output behaviour was detected and reported way before the report generation, e.g. when building the history array. Also, the above function can be defined and documented to give people a way to change single-target loss functions to multi-target ones.

@ablaom
Copy link
Member

ablaom commented Sep 6, 2022

Thanks for this explanation. It seems weird that the flawed custom loss did not break the optimization. I think that is worth investigating. I will open an issue.

Also, the above function can be defined and documented to give people a way to change single-target loss functions to multi-target ones.

Yes, adding wrappers for multi-target losses is on the list: JuliaAI/MLJBase.jl#502

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants